Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

process: make process.nextTick() awaitable #38393

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

process: make process.nextTick() awaitable #38393

wants to merge 2 commits into from

Conversation

XadillaX
Copy link
Contributor

@XadillaX XadillaX commented Apr 25, 2021 •
edited
Loading

For code changes:

  • Include tests for any bug fixes or new features.
  • Update documentation if relevant.
  • Ensure that make -j4 test (UNIX), or vcbuild test (Windows) passes.

@XadillaX XadillaX requested a review from joyeecheung April 25, 2021 03:29
mscdex
mscdex reviewed Apr 25, 2021
View reviewed changes
lib/internal/process/task_queues.js Outdated
let promise;

if (!arguments.length) {
const defered = createDeferredPromise();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const defered = createDeferredPromise();
const deferred = createDeferredPromise();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

mscdex
mscdex reviewed Apr 25, 2021
View reviewed changes
lib/internal/process/task_queues.js Outdated
const defered = createDeferredPromise();
promise = defered.promise;
const { resolve } = defered;
callback = nextTickDefaultCallback.bind(undefined, resolve);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could be simplified without the destructuring assignment above this line along with:

Suggested change
callback = nextTickDefaultCallback.bind(undefined, resolve);
callback = nextTickDefaultCallback.bind(undefined, deferred.resolve);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

devsnek
devsnek suggested changes Apr 25, 2021
View reviewed changes
Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should exist, it's basically "run at the end of the microtask queue after the next tick", which is not an idiom anyone should be reaching for.

@XadillaX
Copy link
Contributor Author

XadillaX commented Apr 25, 2021 •
edited
Loading

I don't think this should exist, it's basically "run at the end of the microtask queue after the next tick", which is not an idiom anyone should be reaching for.

It just for await things for next tick (or anything else in next event loop).

So:

const deferred = createDeferredPromise();
setTimeout(() => { derferred.resolve; }, 0);
await deferred.promise;

// .. do  something in next (or next next ...) tick

may be OK too.

@targos targos added process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. promises Issues and PRs related to ECMAScript promises. labels Apr 25, 2021
@targos
Copy link
Member

targos commented Apr 25, 2021

What use case would be solved by this that cannot work with setImmediate ?

import { setImmediate } from 'timers/promises';
await setImmediate()

@XadillaX
Copy link
Contributor Author

What use case would be solved by this that cannot work with setImmediate ?

import { setImmediate } from 'timers/promises';
await setImmediate()

I mean just like setImmediate(), we make process.nextTick() promisified too.

@aduh95
Copy link
Contributor

aduh95 commented Apr 25, 2021 •
edited
Loading

This is similar to Promise.resolve(arg).then(cb), correct? If so, I think we should not encourage users to use a Node.js specific API.
Related discussion: #19617 (as said in this thread, process.nextTick is already awaitable, technically)

benjamingr
benjamingr requested changes Apr 25, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also -1, if you want a version of proces.nextTick that returns a Promise you can just await null or await Promise.resolve().

If anything ideally we'll deprecate process.nextTick in favour of the web standard queueMicrotask

@benjamingr
Copy link
Member

It just for await things for next tick (or anything else in next event loop).

Note that nextTick is opposite of "in the next tick". It's "in the current tick but after all synchronous code and previously queued nextTicks finished".

setImmediate is the opposite of immediately - it runs in the next tick 😅 .

Their naming is pretty confusing. setImmediate is already promisified and people can use that to await for I/O to run.

jasnell
jasnell requested changes Apr 25, 2021
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm generally -1 on this. There are already a number of ways of doing similar deferals...

await Promise.resolve();

// or 

await new Promise((resolve) => process.nextTick(resolve));

// or, with slightly different timing semantics...

const { setTimeout } = require('timers/promises');
await setTimeout();

Let's leave process.nextTick() unchanged. We don't need Promise APIs for everything.

@benjamingr
Copy link
Member

@jasnell technically #1 and #2 have different timings as well since they don't run on the same queue which is exactly the sort of implementation detail users shouldn't care about.

@jasnell
Copy link
Member

jasnell commented Apr 25, 2021

Yep. Making nextTick awaitable just makes things even weirder.

@joyeecheung
Copy link
Member

joyeecheung commented Apr 27, 2021 •
edited
Loading

I am -1 as well..I think this would just make things even more complex for our users, with no obvious gains

@wzhouwzhou
Copy link

As just an opinion from userland, a general -1 to bloating up and adding unnecessary complexity for minimal gain and usage. In terms of performance, this PR introduces additional quaint checks that hog the EL and you can always use any of the aforementioned solutions for some specialised use cases. Copying in @tbnritzdoge as well who is also familiar with async overhead.

Vetlix
Vetlix suggested changes May 1, 2021
View reviewed changes
Copy link

@Vetlix Vetlix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removal of all edits here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@Vetlix Vetlix Vetlix requested changes

@devsnek devsnek devsnek requested changes

@benjamingr benjamingr benjamingr requested changes

@jasnell jasnell jasnell requested changes

@joyeecheung joyeecheung Awaiting requested review from joyeecheung

Assignees
No one assigned
Labels
process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@XadillaX @targos @aduh95 @benjamingr @jasnell @joyeecheung @wzhouwzhou @mscdex @devsnek @Vetlix